fix(ui): bail on fetchMore recursion when no new items#2606
fix(ui): bail on fetchMore recursion when no new items#2606ghostdevv merged 1 commit intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughA bug fix in the Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/composables/npm/useSearch.ts`:
- Around line 284-288: When the bail condition (if ((cache.value?.objects.length
?? 0) === beforeCount) return) triggers we must mark a local "exhausted" (or
"capped") flag so callers see no more results; set exhausted = true at that bail
site and include exhausted in the computed hasMore so hasMore = !exhausted &&
(cache.value.objects.length < total) (or similar). Also reset exhausted to false
whenever the query or provider changes (the places that refresh the search,
e.g., the functions that update query/filters or re-run the provider) so a new
search can retry, and apply the same change to the other identical bail at lines
337-340 (same cache/beforeCount check). Ensure you reference and update the
local exhausted variable wherever hasMore is computed and where searches are
reinitialized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: be1acff2-fa2d-4d8d-8c47-2294c559ad89
📒 Files selected for processing (1)
app/composables/npm/useSearch.ts
| // Bail if the provider gave us no new unique items | ||
| // Without something like this the recursion below never terminates. | ||
| if ((cache.value?.objects.length ?? 0) === beforeCount) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Propagate the no-growth state to hasMore.
The new bail stops the recursive call, but hasMore still stays true when objects.length < total after an Algolia cap response. That leaves callers free to immediately request more and repeat the same empty fetch. Track a local “exhausted/capped” state, reset it on query/provider refreshes, and include it in hasMore.
Suggested shape
const isLoadingMore = shallowRef(false)
const isRateLimited = shallowRef(false)
+ const isFetchExhausted = shallowRef(false)
...
cache.value = null
+ isFetchExhausted.value = false
...
if ((cache.value?.objects.length ?? 0) === beforeCount) {
+ isFetchExhausted.value = true
return
}
+ isFetchExhausted.value = false
...
const hasMore = computed(() => {
if (!cache.value) return true
+ if (isFetchExhausted.value) return false
return cache.value.objects.length < cache.value.total
})Also applies to: 337-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/composables/npm/useSearch.ts` around lines 284 - 288, When the bail
condition (if ((cache.value?.objects.length ?? 0) === beforeCount) return)
triggers we must mark a local "exhausted" (or "capped") flag so callers see no
more results; set exhausted = true at that bail site and include exhausted in
the computed hasMore so hasMore = !exhausted && (cache.value.objects.length <
total) (or similar). Also reset exhausted to false whenever the query or
provider changes (the places that refresh the search, e.g., the functions that
update query/filters or re-run the provider) so a new search can retry, and
apply the same change to the other identical bail at lines 337-340 (same
cache/beforeCount check). Ensure you reference and update the local exhausted
variable wherever hasMore is computed and where searches are reinitialized.
There was a problem hiding this comment.
What the bot is pointing out is not introduced by this PR, the hasMore already is always true for this case of search results w/ more than 1k entries. I'll add it to the caveats though, it should be handled in a followup IMO.
There was a problem hiding this comment.
@jonchurch, that's a fair point — hasMore staying true past Algolia's pagination cap is indeed pre-existing behavior independent of this PR's changes. The bail added here prevents the freeze; the stale hasMore state is a separate UX concern. Tracking it as a follow-up makes sense. Thanks for the context!
🧠 Learnings used
Learnt from: alexdln
Repo: npmx-dev/npmx.dev PR: 1358
File: app/composables/useSettings.ts:118-131
Timestamp: 2026-02-11T00:34:43.263Z
Learning: In app/composables/useSettings.ts, the search provider cookie getter intentionally checks only for 'npm' explicitly and falls back to settings for all other cases, because 'algolia' is the default search provider. This treats the cookie as an override flag specifically for the non-default option.
Learnt from: Kiwow
Repo: npmx-dev/npmx.dev PR: 2506
File: app/pages/compare.vue:0-0
Timestamp: 2026-04-13T22:26:42.895Z
Learning: In `app/composables/usePackageComparison.ts`, `packagesData` is defined as `computed(() => packages.value.map(name => cache.value.get(name) ?? null))`, which means it is **always** the same length as `packages.value`. Unloaded/missing packages yield `null` entries rather than a shorter array. Therefore, checking `packagesData.value.length >= 1 && packagesData.value.every(data => data !== null)` is sufficient to confirm that all selected packages have loaded. Do not suggest adding a `packagesData.value.length === packages.value.length` guard, as this invariant is structurally guaranteed.
Learnt from: ShroXd
Repo: npmx-dev/npmx.dev PR: 2115
File: app/pages/search.vue:365-368
Timestamp: 2026-03-17T06:53:49.002Z
Learning: In npmx-dev/npmx.dev (app/pages/search.vue), the `showClaimPrompt` computed intentionally returns `true` during a `status === 'pending'` fetch even when `avail.name` doesn't match `committedQuery.value.trim()`. This is by design to prevent the claim prompt from flickering/disappearing while the user is typing (avoiding layout shifts). The claim button is separately guarded with `:disabled="status === 'pending'"` to prevent accidental clicks on stale data. Do not suggest adding an `avail.name === committedQuery` guard to the pending branch.
Learnt from: danielroe
Repo: npmx-dev/npmx.dev PR: 1204
File: app/composables/npm/useUserPackages.ts:40-41
Timestamp: 2026-02-08T13:24:06.104Z
Learning: In Nuxt 3, when using useAsyncData or useLazyAsyncData, allow and rely on reactive keys by typing them as MaybeRefOrGetter<string>. Keys can be a string, a Ref<string>, a ComputedRef<string>, or a getter () => string. Reactive keys should automatically trigger refetches when their value changes. In reviews, verify that data scripts pass a MaybeRefOrGetter<string> (not a plain string-only key if reactivity is intended), and that the logic handles dynamic key changes without stale data.
Learnt from: MatteoGabriele
Repo: npmx-dev/npmx.dev PR: 1922
File: app/composables/usePackageListPreferences.ts:52-53
Timestamp: 2026-03-05T10:14:50.799Z
Learning: In Nuxt projects (here npmx-dev/npmx.dev), exports from shared/types/* (e.g., PAGE_SIZE_OPTIONS, DEFAULT_COLUMNS, DEFAULT_PREFERENCES, PageSize) are auto-imported by Nuxt for composables and components. Do not add explicit import statements for these constants/types when using files under shared/types/, and rely on the auto-imported bindings in files under app/composables (and similarly in components). This pattern applies to all TS files within app/composables that reference these shared/types exports.
There was a problem hiding this comment.
Could you create/update an issue for the follow up so we don't forget?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
graphieros
added
the
needs review
ghostdevv
removed
the
needs review
This sounds similar to the PR that is open now #1996 - could you take a look at see how similar the approach is? Maybe we can combine efforts |
3 participants
🔗 Linked issue
closes #2305
🧭 Context
Problem
Scrolling past ~1000 results on the
/searchview freezes/crashes the browser tab. My Chrome task manager showed it pegged at ~120% CPU.Repro:
/search?q=reactin an incognito tab (to use the defaults aka algolia search provider, infinite scroll)Root cause
tldr;
fetchMorerecurses without a termination check for "server returned nothing new." When Algolia returns empty hits past its 1000 result cap, the recursion never exits, and each cycle's Vue reactivity flush pegs the main thread.fetchMoreinapp/composables/npm/useSearch.tsrecurses while the following condition is true:Algolia enforces a
paginationLimitedTocap of 1000 on thenpm-searchindex (it's just their default, it could be tweaked in the future). As in, regardless of how many results are available, you can neveroffset=your way past 1k results.The npmx bug, though, is that algolia returns an empty array of "hits"
{ hits: [], nbHits: 64812, ... }so the condition above will always be true, we can never break out of the recursion as our result set will always be less than the total reported from the response.So we recurse. Since every iteration uses the same params (
currentCountstayed at 1000), the algolia client's in-memory response cache returns instantly, which lets the recursion turn into a tight nearly synchronous loop. Each iteration reassigns the reactive refcache.value, triggering vue'sflushJobswhich does synchronous work on the main thread. 💥 pegged main thread.📚 Description
Solution
Bail out when a fetch fails to grow the merged cache:
This fix is defensive, it doesn't assume any specific reason the server stopped returning items. If a fetch produced no progress, stop asking, bail out of the infinite recursion.
After this PR, you can scroll all the way to page 40 without the tab crashing. No further though, bc by then the server has stopped returning results (default page size 25 * 40 = 1k).
Caveats!
This PR only stops the freeze. It does not fix the UI, which will still tell users there are more results, but sadly they just can't access them. That's a fine tradeoff as a short term, bc right now npmx.dev can freeze your browser tab which is a way worse experience.
Here's what I mean about the lying UX, it lists 400k packages for "react" but you can only view 1k, and in table view it shows lots of pages that will just be empty. That's exactly how it is today, just without freezing the site.
and the table view...
Follow-up Fix
Keeping this PR scoped to just the freeze. I have a follow-up staged that reads Algolia's
paginationLimitedTocap dynamically from the response (nbPages * hitsPerPagegives it to you whennbHitsis over cap), so we don't hardcode 1000. Ifnpm-searchever bumps the cap, npmx picks it up for free.There's a UX wrinkle worth hashing out separately though. Once the total is honest, broad queries will just say "1,000 packages" without any hint that there are actually 100k+ matches out there. Whether we signal that is a judgment call, and I'd rather not bundle it in here.